Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(crush): support dot in property name #23

Closed
wants to merge 7 commits into from

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Jun 26, 2024

Tip

The owner of this PR can publish a preview release by commenting /publish in this PR. Afterwards, anyone can try it out by running pnpm add radashi@pr<PR_NUMBER>.

Description

Currently, the crush function combines the objectify and keys functions to achieve its behavior.

The cause of the issue is with keys creating no discernible difference between a property with a dot in it (i.e. {"a.b":1} => "a.b") and a property path of multiple property names (i.e. {a:{b:1}} => "a.b").

As of now, this PR only adds a test case to verify the bug report by @stefaanMLB.

Checklist

  • Changes are covered by tests if behavior has been changed or added
  • Tests have 100% coverage
  • If code changes were made, the documentation (in the /docs directory) has been updated

Resolves

Resolves sodiray/radash#365

Bundle impact

Status File Size Difference (%)
M src/object/crush.ts 2951 -396 (-58%)

Footnotes

  1. Function size includes the import dependencies of the function.

@aleclarson aleclarson added help wanted Extra attention is needed upstream fix Fixes a bug that existed in Radash labels Jun 26, 2024
@aleclarson aleclarson force-pushed the main branch 4 times, most recently from 35fc67a to 04bad7b Compare July 1, 2024 19:17
@aleclarson aleclarson marked this pull request as draft July 3, 2024 18:08
@stefaanMLB
Copy link

stefaanMLB commented Jul 5, 2024

@aleclarson, I fixed the problem but I'm having trouble with the tooling.
Can I get away with using NPM or is there no way around using PNPM ?
After adapting the code I get a lot of errors in the async module which I didn't change 🤔
This is the code that fixes the crush problem, not sure how to proceed from here

type Primitive = number | string | boolean | Date | symbol | bigint | undefined | null;

const crush = <TValue extends object>(value: TValue): Record<string, Primitive> | Record<string, never> => {
  if (!value) return {};
  const crushToPvArray: (obj: object, path: string) => Array<{ p: string; v: Primitive }> = (
    obj: object,
    path: string
  ) =>
    Object.entries(obj).flatMap(([key, value]) =>
      isPrimitive(value) ? { p: path ? `${path}.${key}` : key, v: value } : crushToPvArray(value, `${path}.${key}`)
    );

  return objectify(
    crushToPvArray(value, ''),
    (o) => o.p,
    (o) => o.v
  );
};

@aleclarson
Copy link
Member Author

Hi @stefaanMLB,

If PNPM is causing you trouble locally, you could try using Github codespaces instead. In my experience, PNPM hasn't been hard to use, but I don't know your set up.

If you'd prefer to use NPM, it should be okay, but you don't get the lockfile (which normally isn't a big deal).

npm install --no-package-lock

Feel free to open a new PR with your solution and I'll review it there, thanks.

stefaanv added a commit to stefaanv/radashi that referenced this pull request Jul 8, 2024
@stefaanMLB
Copy link

@aleclarson, I believe I solved this problem and succeeded to create PR#95.
I'm not accustomed to working with PR's, so please let me know if I did everything correct.
If this one is OK, then I'll be happy to contribute more to the library.

@aleclarson aleclarson force-pushed the main branch 2 times, most recently from 2154f96 to 6a4b4f6 Compare July 12, 2024 00:23
@aleclarson aleclarson force-pushed the fix/crush-dot-property branch from e789c1f to 0168241 Compare July 19, 2024 17:44
@aleclarson
Copy link
Member Author

Closed in favor of #95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed upstream fix Fixes a bug that existed in Radash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crush() doesn't handle dots in property names well
3 participants